Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[homekit] Add workaround for color picker #7491

Closed
wants to merge 1 commit into from
Closed

[homekit] Add workaround for color picker #7491

wants to merge 1 commit into from

Conversation

jeremystucki
Copy link
Member

HomeKit sends saturation and hue separately. This leads to two conflicting commands being sent.

20:48:13.238 [WARN ] [essories.AbstractHomekitAccessoryImpl] - Changing saturation to 5.0
20:48:13.250 [WARN ] [essories.AbstractHomekitAccessoryImpl] - Changing hue to 63.0
20:48:13.272 [INFO ] [smarthome.event.ItemCommandEvent     ] - Item 'Homekit_Test' received command 222,5,100
20:48:13.281 [INFO ] [arthome.event.ItemStatePredictedEvent] - Homekit_Test predicted to become 222,5,100
20:48:13.354 [INFO ] [smarthome.event.ItemCommandEvent     ] - Item 'Homekit_Test' received command 63.0,44,100
20:48:13.361 [INFO ] [smarthome.event.ItemStateChangedEvent] - Homekit_Test changed from 222,44,100 to 222,5,100
20:48:13.367 [INFO ] [arthome.event.ItemStatePredictedEvent] - Homekit_Test predicted to become 63.0,44,100
20:48:13.391 [INFO ] [smarthome.event.ItemStateChangedEvent] - Homekit_Test changed from 222,5,100 to 63.0,44,100

The workaround is to just do nothing when the saturation is set and only send a command once the hue is set. This is not ideal, but it seems to work a lot better now.

I am using hue lights and nanoleaf panels.

@TravisBuddy
Copy link

Travis tests were successful

Hey @jeremystucki,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@J-N-K J-N-K mentioned this pull request May 1, 2020
Merged
@yfre
Copy link
Contributor

yfre commented May 1, 2020

@jeremystucki im trying to reproduce your behaviour. can you show how corresponding items are defined in your configuration? is one item or several?
how do you change the saturation - via openhab UI, home app?

@jeremystucki
Copy link
Member Author

@yfre

I have a color item with one connected hue bulb. My network connection is quite poor so that could be contributing to the issue.

The saturation is updated together with the hue.

here is a video of the issue: https://imgur.com/mTyVhVX

@J-N-K
Copy link
Member

J-N-K commented May 5, 2020

Could you please check if - after the latest refactorings - the problem still persists? Thanks.

@J-N-K J-N-K added the awaiting feedback Awaiting feedback from the pull request author label May 20, 2020
@yfre
Copy link
Contributor

yfre commented May 25, 2020

@J-N-K @jeremystucki
i checked yesterday. The issue is still there.
i see 3 options:

  • ignore saturation. we will lose part of the information sent from home app. especially if only saturation was changed.
  • use setState(state) in addition to send(command). this solves the issue as well but would potential trigger wrong events for rules.
  • we wait a short time, e.g. 20ms, collect both changes - HUE and Saturation and update them with one "send(command)". probably the best but most complex option.

@jeremystucki
Copy link
Member Author

jeremystucki commented May 25, 2020 •
edited
Loading

I tried implementing a wait time, but then you can't drag your finger on the color wheel, as it sends too many commands in a short period of time.

I would prefer the setState(state) option.

@yfre
Copy link
Contributor

yfre commented May 26, 2020

I tried implementing a wait time, but then you can't drag your finger on the color wheel, as it sends too many commands in a short period of time.

I would prefer the setState(state) option.

good to know that delay/wait has impact on the home app UI.
the homekit binding implementation has changed slightly since this PR. so, i will adapt the setState approach to the new implementation and create a new PR

@yfre yfre mentioned this pull request May 31, 2020
Merged
@jeremystucki
Copy link
Member Author

Closing in favor of #7825

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DaRe92 DaRe92 DaRe92 approved these changes

Assignees
No one assigned
Labels
awaiting feedback Awaiting feedback from the pull request author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jeremystucki @TravisBuddy @yfre @J-N-K @DaRe92